Skip to content

refactor(pagination): one shared Pagination component for all six surfaces#477

Open
rusikv wants to merge 4 commits into
thingsboard:developfrom
rusikv:feat/iot-hub-pagination
Open

refactor(pagination): one shared Pagination component for all six surfaces#477
rusikv wants to merge 4 commits into
thingsboard:developfrom
rusikv:feat/iot-hub-pagination

Conversation

@rusikv

@rusikv rusikv commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

What

Extracts pagination into a single shared component — src/components/Pagination/ — and migrates every surface that paginates to it:

Surface Before After
IoT Hub category / search / creator PaginationLink + PaginationChevron + iot-hub-pagination-update.ts shared component (link mode + dynamic rebuild)
Device Library + Partners hardware shared dumb shell with singleton IDs; windowing JS copy-pasted into both consumers shared component (interactive mode)
Case studies index bespoke innerHTML renderer, cs-* classes shared component (interactive mode)
Blog index numbers-only innerHTML renderer (no prev/next) shared component (interactive mode)

Net: −389 lines, one windowing algorithm instead of three drifted copies, one styling vocabulary instead of four.

Design

  • Two modes, one prop. With basePath, page numbers are real anchors (/iot-hub/widgets/3/, rel=prev/next, crawlable, zero JS). Without it, buttons dispatch bubbling tb-pagination:page-change events and updatePagination() rebuilds the nav. IoT Hub uses both in sequence: static links until search/filters activate the dynamic pipeline.
  • Consumers keep their own behavior. URL state (blog ?page=), scroll handling, sessionStorage restore (case-studies), API refetching (IoT Hub) all stay in consumer code — the component only renders and reports. This boundary is documented in a design-contract comment at the top of the component.
  • IoT-Hub-only extras compose in via a controls slot (PerPageSelector.astro, the items-per-page dropdown). Slotting instead of a prop keeps the dropdown's script/styles out of the other surfaces' bundles — Astro bundles per import graph, not per render.

Improvements over the old implementations

  • Keyboard focus is restored to the equivalent control after every rebuild (all old surfaces dropped focus to <body>; the old case-studies code could even double-fire prev/next).
  • aria-current="page" everywhere (previously IoT Hub only); icon buttons have aria-labels at all widths; prefers-reduced-motion respected.
  • Brand tokens (--color-brand/-contrast) replace hardcoded #b3c7ff/#17181c/--sl-* colors → correct dark theme on all surfaces.
  • Deterministic SSR ids (reproducible builds); compact "Page X of Y" + chevrons below the lg breakpoint on every surface.
  • Multiple instances per page are now safe (the old Device Library shell was a fixed-ID singleton).

Intended visual/behavior deltas (not regressions)

  • Unified look: 40px hit targets, brand-accent current-page pill, chevron prev/next.
  • Blog gains prev/next buttons (it had none).
  • Case studies / Device Library / Partners: text "Previous/Next" → chevrons; full number rows up to 7 pages (was 5).
  • Device Library / Partners: the redundant always-visible "Page X of Y" line is dropped (the compact sub-lg layout shows it).
  • IoT Hub dark mode: current-page text is navy-on-lavender per brand tokens (was near-black).

Verification

  • astro check (0/0/0), ESLint clean on all touched files.
  • Before/after headless-Chrome screenshots of all six surfaces: IoT Hub pixel-identical (incl. page-3 current state); others show only the intended deltas.
  • Deep-linked /blog/?page=2 exercises the client updater + URL restore end-to-end.
  • Repo-wide greps confirm zero references to the deleted components, classes, or old event names.
  • Adversarially reviewed; findings fixed (focus restore, deterministic ids, dead code removal).

lint:linkcheck not yet run (no routes/links changed by this PR — pagination URLs are generated by the same paginate()/getStaticPaths code as before).

Six surfaces each carried their own pagination: IoT Hub's link-based
PaginationLink + runtime updater, a button shell shared by Device
Library and Partners (with the windowing JS copy-pasted into both),
and bespoke innerHTML renderers on the blog and case-studies indexes.
Three drifted copies of the windowing algorithm, four styling
vocabularies, and recurring a11y gaps.

Replace all of it with src/components/Pagination/:

- Pagination.astro — one nav, two modes: basePath renders real
  anchors (SSG, rel=prev/next, crawlable); without it, buttons that
  dispatch bubbling tb-pagination:page-change events. Numbered list
  on desktop, compact 'Page X of Y' row below lg. Brand tokens
  (indigo/lavender current-page pill), aria-current in both modes,
  focus-visible rings.
- pagination-client.ts — updatePagination() rebuilds the lists for
  client-driven surfaces; restores keyboard focus to the equivalent
  control after each rebuild (fixes a focus-loss bug all the old
  implementations shared).
- pagination-shared.ts — the single windowing algorithm + strings.
- PerPageSelector.astro — the IoT-Hub-only items-per-page dropdown,
  composed in via the 'controls' slot from consumer files so its
  script/styles ship only to IoT Hub pages (Astro bundles per import
  graph, not per render).

Consumers keep what is theirs: URL state (?page= on blog), scroll
behavior, sessionStorage restore (case-studies), and the dynamic
search pipeline (IoT Hub) — the component only renders and reports.

Intended deltas: unified look on all surfaces, blog gains prev/next
chevrons, grid surfaces show full number rows up to 7 pages (was 5),
Device Library/Partners drop the redundant always-visible
'Page X of Y' line (lives in the sub-lg compact layout now).
@rusikv rusikv marked this pull request as ready for review June 12, 2026 12:09
@rusikv rusikv changed the base branch from feature/iot-hub-page to develop June 15, 2026 13:10
@rusikv rusikv requested a review from vvlladd28 June 15, 2026 13:13

@vvlladd28 vvlladd28 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary

Reviewed 16 changed files in refactor(pagination): one shared Pagination component for all six surfaces. Left 6 comment(s) inline.

The refactor is clean and well-documented: the link/interactive mode split is coherent, buildPages/chevron geometry is centralized in pagination-shared.ts so static and runtime renders can't drift, totalPages is clamped to >= 1, the deep-link ?page= path and the double-init guards check out, and repo-wide greps confirm no dangling references to the deleted components, classes, or old event names. The comments below are mostly design/maintainability observations rather than correctness defects — one minor focus-management nuance, the rest about coupling and API discoverability.


This review was auto-generated. Findings may contain errors — please verify before applying changes.

...nav.querySelectorAll<HTMLElement>(`[data-nav-role="${role}"]`),
nav.querySelector<HTMLElement>('.tb-pagination__page.is-current'),
].filter((el): el is HTMLElement => !!el);
candidates.find(visible)?.focus();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When prev/next is clicked to the first/last page, that chevron is rebuilt as disabled (is-disabled + aria-disabled) but is still a focusable, still-visible <button>, so candidates.find(visible) parks focus right back on it — a control the user can no longer activate. Since the candidate list already falls through to .tb-pagination__page.is-current, would it be worth skipping disabled candidates here (e.g. filtering out .is-disabled) so boundary navigation lands focus on the current-page button instead of a dead chevron?

class: className = '',
} = Astro.props;

const isLinkMode = typeof basePath === 'string';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link vs. interactive mode is selected implicitly by whether basePath happens to be passed. That's a meaningful behavioral fork — real anchors + SSR navigation vs. event-emitting buttons rebuilt by JS — hinging on one optional prop's presence, and it's easy to get subtly wrong from a call site: a consumer that means to be interactive but passes a basePath silently gets anchors and no events. Would an explicit mode: 'link' | 'interactive' prop (with basePath required only for link mode) make the contract more discoverable? The doc comment is good; it's just that the type signature itself doesn't express the coupling.

<ul class="tb-pagination__pages tb-pagination__pages--numbers">
<li>
{
isLinkMode && hasPrev ? (

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prev/next chevron branch (link <a> / interactive <button> / disabled <span>) is written out four times in the template — prev and next, each duplicated across the --numbers list and the --compact row — differing only in direction, rel, label, and data-nav-role. A small local helper (a function returning the fragment, or an inline sub-component parameterised by direction/target/enabled) would collapse the four near-identical copies into one and keep the variants from drifting. Not load-bearing, but it's the densest part of the file to read.

}

// Same geometry as Chevron.astro (24px viewBox, 2px round stroke).
export const CHEVRON_PATHS = {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two path strings duplicate paths.left/paths.right in Chevron.astro. The comment honestly flags the coupling, but a comment won't keep them in sync — if the icon is ever redrawn, the runtime-built chevrons here drift from the build-time <Chevron> ones that Pagination.astro renders. Since Chevron.astro is essentially a thin wrapper over a path map, could that map be hoisted into a shared TS module that both Chevron.astro and this file import, rather than re-declaring the d-strings?

perPageRoot.querySelectorAll<HTMLButtonElement>('[data-per-page-option]').forEach((opt) => {
const isSelected = opt === target;
opt.classList.toggle('iot-hub-pagination__per-page-option--selected', isSelected);
opt.classList.toggle('tb-pagination__per-page-option--selected', isSelected);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

applyPageSizeToUi reaches into PerPageSelector's internal DOM contract — [data-per-page-option], [data-per-page-value], [data-per-page-label], and this tb-pagination__per-page-option--selected class — to reflect a restored ?pageSize= back into the dropdown. PerPageSelector already owns that markup and does the identical class/aria/label toggling in its own option-click handler, so the selection logic now lives in two places and a rename on either side silently breaks URL-restore here with no type safety to catch it. Could PerPageSelector expose a small imperative helper (e.g. setPerPageValue(root, n)) or react to a tb-pagination:set-page-size event so consumers don't hand-poke its internals? (It does mirror the existing applySortToUi/IotHubSort pattern, so it's at least locally consistent.)

renderResults(items);
if (paginationNav) {
updatePaginationDynamic(paginationNav, { currentPage, totalPages });
updatePagination(paginationNav, { currentPage, totalPages });

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Five of the six interactive consumers call updatePagination with hideOnSinglePage: true; this IoT Hub path omits it and instead leans on the SSR page.lastPage > 1 guard plus showFetchError toggling nav.hidden by hand. The effect differs: if a runtime filter narrows results to a single page here, the nav stays visible showing just "1" rather than hiding like the other surfaces. Intentional asymmetry, or just an omission? Either way, a one-line comment at this call site noting the deliberate divergence from the shared default would save the next person from "fixing" it to match.

@vvlladd28 vvlladd28 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary

Reviewed 16 changed files in refactor(pagination): one shared Pagination component for all six surfaces. Left 6 comments inline.

This is a clean, well-executed dedup refactor. Repo-wide greps confirm no dangling references to the removed components, events (iot-hub-page:change/iot-hub-page-size:change), classes, or strings; the blog's totalPages is correctly recomputed in render(); and the two consumer-side changes (the case-studies double-init guard and the focus-restore on rebuild) are genuine improvements over the old code. I didn't find functional bugs — the inline comments are all maintainability / consistency / API-shape observations on the new shared component.


This review was auto-generated. Findings may contain errors — please verify before applying changes.

}

// Same geometry as Chevron.astro (24px viewBox, 2px round stroke).
export const CHEVRON_PATHS = {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two path strings are a verbatim copy of the left/right entries in Chevron.astro's path map, and the comment right above ("Same geometry as Chevron.astro") spells out exactly the copy-paste invariant this PR is otherwise trying to eliminate. The SSR path already renders <Chevron> directly — only the runtime rebuild in pagination-client.ts needs the raw strings. Could the geometry come from a single source (e.g. Chevron.astro importing these paths, or both reading one shared constant), so a future tweak to the icon can't silently drift between the static and dynamic renders?

right: 'm9 6 6 6-6 6',
} as const;

export function formatPageSummary(current: number, total: number): string {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every other user-facing string here is either centralised in PAGINATION_STRINGS or overridable via props (ariaLabel, prevLabel, nextLabel), but the compact "Page X of Y" summary is hardcoded English with no override hook. On a 14-language site this becomes the one pagination label that silently stays English in the sub-lg layout. Worth routing it through PAGINATION_STRINGS as a template (or accepting it as a prop) for consistency with the rest of the component.

class: className = '',
} = Astro.props;

const isLinkMode = typeof basePath === 'string';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link-vs-interactive mode is selected implicitly by whether basePath is set, and several props only make sense in one mode — hidden is interactive-only per its own doc comment, and currentPage/totalPages are effectively meaningless in interactive mode since the client rebuilds them at runtime. Nothing in the Props type signals which props pair with which mode, so it's easy to pass a combination that does nothing. Grouping the props by mode in the doc comments — or a discriminated mode prop — would make the two-mode design harder to misuse.

<ul class="tb-pagination__pages tb-pagination__pages--numbers">
<li>
{
isLinkMode && hasPrev ? (

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prev/next chevron is authored four times in this template: a 3-way (link / interactive button / disabled span) branch each for prev and next in the numbers list, plus two more 2-way branches in the compact row. They differ only in direction, target page, and label, so it's easy for the variants to drift — e.g. someone fixing an a11y attribute on one and missing the other three. A small local renderChevron(direction, enabled, hrefOrRole, label) fragment would collapse most of this duplication.


function restoreFocus(nav: HTMLElement, role: string | null): void {
if (!role) return;
const visible = (el: HTMLElement) => el.offsetParent !== null;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two notes on restoreFocus:

  1. offsetParent !== null is a leaky visibility probe — it works here only because the numbers/compact rows are toggled with display: none via media queries. If a future layout variant ever hides a row with visibility/opacity/clip instead, the desktop-vs-mobile focus pick would break silently. Worth a one-line comment pinning down the display:none-only assumption.
  2. When the user pages to the last (or first) page via a chevron, that chevron has just become is-disabled — but it's still focusable (no disabled attribute, only aria-disabled + pointer-events: none), so focus is restored onto a dimmed, non-interactive control. Since data-nav-role matches before the .is-current fallback, that boundary case lands focus on the disabled chevron rather than the current page. Preferring the current-page button when the captured role resolves to a disabled control might feel less surprising.

perPageRoot.querySelectorAll<HTMLButtonElement>('[data-per-page-option]').forEach((opt) => {
const isSelected = opt === target;
opt.classList.toggle('iot-hub-pagination__per-page-option--selected', isSelected);
opt.classList.toggle('tb-pagination__per-page-option--selected', isSelected);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

applyPageSizeToUi re-implements PerPageSelector's own selection logic — the tb-pagination__per-page-option--selected class toggle, aria-pressed, label text, and the data-per-page dataset are all also owned by setupPerPage inside the component. So two files now have to agree on PerPageSelector's private class names and DOM contract, which cuts against the otherwise-clean black-box boundary (events + data attributes) the shared component establishes. Exposing a small helper like setPerPageValue(root, n) from the component would keep that markup owned in exactly one place.

rusikv added 3 commits June 18, 2026 11:40
- Decouple PerPageSelector from the nav: new PaginationBar wraps the nav
  and the selector as siblings, so hiding the nav on a single page (or
  fetch error) no longer strips the items-per-page control — closes the
  trap where raising page size to one page removed the only way to lower
  it again. (review thingsboard#5)
- Enable hideOnSinglePage on the IoT Hub runtime path now that the
  selector survives the nav hiding; route error-hide to the bar. (thingsboard#6)
- Extract per-page-client.ts (setPerPageValue/setupPerPage) so URL-restore
  stops re-implementing the selector's internal toggling. (thingsboard#5)
- restoreFocus skips .is-disabled controls so boundary navigation lands on
  the current page, not a dead chevron. (thingsboard#1)
- Collapse the four duplicated prev/next chevron branches into
  PaginationChevron.astro. (thingsboard#3)
- Single chevron path map in chevron-paths.ts, shared by Chevron.astro and
  pagination-shared.ts. (thingsboard#4)
- Guard pageUrl so it only builds a URL in link mode.
Resolves conflicts from develop's Device Library removal (thingsboard#491) vs the
shared-Pagination refactor:
- DeviceLibrary/* removed (accept develop): the feature is gone, so the
  refactor's edits to DeviceLibrary.astro are moot.
- Partners/Pagination.astro deleted: Partners now uses the shared
  Pagination component, not the old per-surface one develop moved here.
- Partners/PartnerLibrary.astro: keep the shared-Pagination migration;
  point SearchBar at the moved ./SearchBar.astro.
- Replace the implicit basePath-presence mode inference with an explicit
  discriminated-union `mode: 'link' | 'interactive'` prop. Illegal combos
  (basePath in interactive mode, hidden in link mode, missing mode) are now
  astro-check errors instead of silent runtime surprises. The runtime markup
  contract and updatePagination are untouched, so IoT Hub's SSR-link ->
  runtime-interactive hybrid is unaffected. (review thingsboard#2)
- Centralize the compact 'Page X of Y' summary into PAGINATION_STRINGS as a
  {current}/{total} template so every user-facing string lives in one place.
- Document restoreFocus's display:none-only assumption behind the offsetParent
  visibility probe.
@rusikv rusikv requested a review from vvlladd28 June 18, 2026 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants